Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Runtime feature detection #13549

Merged
merged 12 commits into from
Jan 21, 2019
Merged

Runtime feature detection #13549

merged 12 commits into from
Jan 21, 2019

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Dec 5, 2018

Description

It would be good to have runtime feature detection, for example to know if we were compiled with OPENCV, LAPACK, some tests fail when MXNet is not compiled with some feature or another.

This is a proposal for a runtime feature detection mechanism that would expose how the library was compiled and what features are available.

It also grooms some of the feature flags which were not uniformly set in base.h

See related discussion in dev:

https://lists.apache.org/thread.html/19fe1da67551f9a199ec85dbb9c894e65c205d5fdd6764509e6e41ed@%3Cdev.mxnet.apache.org%3E

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@nswamy nswamy added the pr-awaiting-review PR is waiting for code review label Dec 7, 2018
@Roshrini
Copy link
Member

@larroy Thanks for working on this. Is this PR ready to review or still WIP?
@leleamol @anirudh2290 Can you review this PR?

@larroy
Copy link
Contributor Author

larroy commented Dec 18, 2018

It's WIP. I'm taking a few days off. Will get back to this after my break.

@Roshrini
Copy link
Member

Roshrini commented Jan 3, 2019

@mxnet-label-bot Update [pr-work-in-progress]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Jan 3, 2019
src/mxfeatures.cc Outdated Show resolved Hide resolved
class Storage {
public:
Storage():
feature_bits()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will feature_bits be initialized without explicit init?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, still better to initialize members explicitly for consistency in C++

src/mxfeatures.cc Outdated Show resolved Hide resolved
feature_bits.set(JEMALLOC);
#endif
}
bool is_enabled(unsigned feat) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: const?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Consistency with enum type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't because of higher level language ergonomics.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

src/mxfeatures.cc Outdated Show resolved Hide resolved
include/mxnet/mxfeatures.h Show resolved Hide resolved
@lebeg
Copy link
Contributor

lebeg commented Jan 7, 2019

You could add the dev lists discussion link to the description.

src/mxfeatures.cc Outdated Show resolved Hide resolved
src/mxfeatures.cc Outdated Show resolved Hide resolved
src/mxfeatures.cc Outdated Show resolved Hide resolved
include/mxnet/mxfeatures.h Outdated Show resolved Hide resolved
@pengzhao-intel
Copy link
Contributor

Thanks for the PR. It's a very useful proposal and improvement.
With these piece of information, I think we can enable more backend specific features in the runtime.
@ZhennanQin

include/mxnet/mxfeatures.h Outdated Show resolved Hide resolved
@larroy larroy changed the title [WIP] Prototype for runtime feature detection [WIP] Runtime feature detection Jan 8, 2019
@larroy larroy force-pushed the feature_discovery branch 3 times, most recently from cadf83e to 7aff860 Compare January 9, 2019 00:53
@larroy larroy changed the title [WIP] Runtime feature detection Runtime feature detection Jan 9, 2019
@larroy
Copy link
Contributor Author

larroy commented Jan 9, 2019

@mxnet-label-bot Update [pr-awaiting-review]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Jan 9, 2019
@larroy
Copy link
Contributor Author

larroy commented Jan 9, 2019

@stsukrov not sure what you mean.

@larroy
Copy link
Contributor Author

larroy commented Jan 17, 2019

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Jan 17, 2019
Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide usage details? Maybe example inputs and outputs of running it?

@larroy
Copy link
Contributor Author

larroy commented Jan 18, 2019

@aaronmarkham where would you suggest to add this? you mean as a documentation? The unit tests show how to use it. We need to build on top of this feature for selecting tests at runtime, so there will be more usage examples.

@aaronmarkham
Copy link
Contributor

I could have just randomly tried executing the scripts but I wanted to know, in advance, what should be run to make them work and what to expect to see if they did work. Then I'd know if it didn't work, or if it did.

@larroy
Copy link
Contributor Author

larroy commented Jan 18, 2019

Did you see the provided unit test?

@aaronmarkham aaronmarkham merged commit 692a24a into apache:master Jan 21, 2019
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Jan 27, 2019
* Prototype for runtime feature detection

* Includes from diamond to quotes

* Add CPU feature and BLAS flavour flags

* Add BLAS flavour and CPU SSE and AVX flags

* MXNET_USE_LAPACK

* Fix C++ linting errors

* Expose runtime feature detection in the public C API and in the Python API

* Refactor Storage -> FeatureSet

* Refine documentation

* Add failure case

* Fix pylint

* Address CR comments
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
* Prototype for runtime feature detection

* Includes from diamond to quotes

* Add CPU feature and BLAS flavour flags

* Add BLAS flavour and CPU SSE and AVX flags

* MXNET_USE_LAPACK

* Fix C++ linting errors

* Expose runtime feature detection in the public C API and in the Python API

* Refactor Storage -> FeatureSet

* Refine documentation

* Add failure case

* Fix pylint

* Address CR comments
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Prototype for runtime feature detection

* Includes from diamond to quotes

* Add CPU feature and BLAS flavour flags

* Add BLAS flavour and CPU SSE and AVX flags

* MXNET_USE_LAPACK

* Fix C++ linting errors

* Expose runtime feature detection in the public C API and in the Python API

* Refactor Storage -> FeatureSet

* Refine documentation

* Add failure case

* Fix pylint

* Address CR comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.